MDEV-37755 ilist::pop_back() removes sentinel #4411
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
vlad-lesin
force-pushed
the
10.6-MDEV-37755
branch
from
fb09b6b to
5620ee3
Compare
dr-m
reviewed
Nov 10, 2025
Contributor
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
Great work!
I would suggest some very minor cleanup to fil_space_free_low() and fil_space_t::drop().
storage/innobase/fil/fil0fil.cc
Outdated
Comment on lines
1595
to
1610
| *detached_handle = handle; | ||
| else | ||
| os_file_close(handle); | ||
|
|
||
| /* The above mnt.commit_file() call should remove the space from | ||
| fil_system.named_spaces, but some pending operations can push it back | ||
| to the container again. */ | ||
| mysql_mutex_lock(&log_sys.mutex); | ||
| if (space->max_lsn != 0) | ||
| { | ||
| space->max_lsn= 0; | ||
| fil_system.named_spaces.remove(*space); | ||
| } | ||
| mysql_mutex_unlock(&log_sys.mutex); | ||
|
|
||
| return space; |
Contributor
There was a problem hiding this comment.
I see that the added code corresponds to the logic in fil_space_free(). It could be good to mention that in a comment. I was also wondering if the code could be added right after the mysql_mutex_unlock(&fil_system.mutex), before we do anything with handle, to make it more like what fil_space_free() is doing.
As far as I understand, we should be able to safely simplify fil_space_free_low() as well:
diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc
index 4e8ddf5f757..08349905b86 100644
--- a/storage/innobase/fil/fil0fil.cc
+++ b/storage/innobase/fil/fil0fil.cc
@@ -861,13 +861,7 @@ static void fil_space_free_low(fil_space_t *space) noexcept
/* The tablespace must not be in fil_system.named_spaces. */
ut_ad(srv_fast_shutdown == 2 || !srv_was_started
|| space->max_lsn == 0);
-
- /* Wait for fil_space_t::release() after
- fil_system_t::detach(), the tablespace cannot be found, so
- fil_space_t::get() would return NULL */
- while (space->referenced()) {
- std::this_thread::sleep_for(std::chrono::microseconds(100));
- }
+ ut_ad(!space->referenced());
for (fil_node_t* node = UT_LIST_GET_FIRST(space->chain);
node != NULL; ) {
@@ -1604,15 +1598,12 @@ fil_space_t *fil_space_t::drop(ulint id, pfs_os_file_t *detached_handle)
pfs_os_file_t handle= fil_system.detach(space, true);
mysql_mutex_unlock(&fil_system.mutex);
- if (detached_handle)
- *detached_handle = handle;
- else
- os_file_close(handle);
-
- /* The above mnt.commit_file() call should remove the space from
- fil_system.named_spaces, but some pending operations can push it back
- to the container again. */
+ /* The above mtr.commit_file(*space, nullptr) should remove the space from
+ fil_system.named_spaces. Before we set the STOPPING_WRITES flag, another
+ concurrent operation could have marked the tablespace dirty again.
+ This clean-up corresponds to fil_space_free(). */
mysql_mutex_lock(&log_sys.mutex);
+ ut_ad((space->pending() & ~NEEDS_FSYNC) == (STOPPING | CLOSING));
if (space->max_lsn != 0)
{
space->max_lsn= 0;
@@ -1620,6 +1611,11 @@ fil_space_t *fil_space_t::drop(ulint id, pfs_os_file_t *detached_handle)
}
mysql_mutex_unlock(&log_sys.mutex);
+ if (detached_handle)
+ *detached_handle = handle;
+ else
+ os_file_close(handle);
+
return space;
}
I tested these changes on top of a merge of 5620ee3 to 759e352, and 200 runs of the test encryption.create_or_replace passed without incident, and so did a run of mysql-test/mtr --suite=encryption,innodb,mariabackup.
Comment on lines
1639
to
1651
| if (fil_space_t *space= fil_space_t::drop(id, &handle)) | ||
| fil_space_free_low(space); | ||
| fil_space_free_low(space); | ||
| return handle; | ||
| } |
Contributor
There was a problem hiding this comment.
The basic indentation offset should be 2 columns, not 4. This looks like an unintentional change that should be omitted.
vlad-lesin
force-pushed
the
10.6-MDEV-37755
branch
from
5620ee3 to
5d528ba
Compare
…amed_spaces mtr.commit_file() call in fil_space_t::drop() removes space from fil_system.named_spaces, but then the space can be inserted in the container again by some another thread while fil_space_t::drop() is waiting for pending operations finishing. The fix is to check and remove a space from fil_system.named_spaces after all pengind operations on the space are finished. Also the ut_d() macro is removed for space->max_lsn=0 assignments to avoid repeated space removing from fil_system.named_spaces. There is error in ilist::pop_back(). ilist::end() returns sentinel, and the pop_back() removes sentinel from the list instead of the last element. The error is fixed in this commit. Reviewed by Marko Mäkelä
vlad-lesin
force-pushed
the
10.6-MDEV-37755
branch
from
5d528ba to
7301fba
Compare
dr-m
approved these changes
Nov 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fix is to remove previous to sentinel node instead of sentinel itself.
Description
TODO: fill description here
Release Notes
TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.
How can this PR be tested?
TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".
If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.
Basing the PR against the correct MariaDB version
mainbranch.PR quality check